-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix gd32e1 #40
Fix gd32e1 #40
Conversation
Hi! Happy new year :) This PR should now be ready for a review. I made changes to the timers that should still work in a similar way, let me know if something is not correct :) |
@@ -2,7 +2,7 @@ | |||
# | |||
# SPDX-License-Identifier: MIT OR Apache-2.0 | |||
|
|||
"TIMER1[3-6]": | |||
CHCTL[01]_Output: | |||
"TIMER1[3456]": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage of this over giving a range like we had before? It's marginally longer, and I find the range easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I read in svdtools, ranges are not supported. This means it was trying to match "TIMER13", "TIMER1-" and "TIMER16" before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? It seemed to be working fine before, and the stm32-rs repository is using it, e.g. in https://github.com/stm32-rs/stm32-rs/blob/master/peripherals/tim/tim_basic.yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it uses this svdtools I don't see any way to use ranges. The parsing code doesn't seem to handle it (https://github.com/stm32-rs/svdtools/blob/074c31f67938ae21efeb2c74fe01260d7e4f55ff/src/patch/mod.rs#L486) and the documentation has a very small set of tokens (https://github.com/stm32-rs/svdtools#name-matching).
It was not working when I was using it, e.g. TIM[1-58]
was matching TIM1, TIM5 and TIM8 (so was not reporting an error not finding TIM- because it only needs one match to work), but not TIM2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using the Python version of svdtools still, so maybe it has more functionality? It does look like there is a bug here though, so I'll have a proper look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried building locally, and I get exactly the same output in the svd/*.svd.patched
files with or without your change to stop using ranges, so I'm pretty sure the svd patch
is handling them properly as expected. Removing my _derivedFrom
fixes does change the output, so we don't want to change that.
I'm not sure why it's not working for you. How are you trying to build it, just using make
? What version of svdtools
do you have installed, and did you install from pip
or somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh, I was using the rust version. The python version uses braceexpand/fnmatch which handles these kind of things. I'll fix that on my machine and try to rollback the other ones I had unrolled in previous versions too then :)
_derivedFrom: "TIMER0.INTF.UPIF.UPIF" | ||
SWEVG: | ||
UPG: | ||
_derivedFrom: "TIMER0.SWEVG.UPG.UPG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't removing all these _derivedFrom
fixes result in the fields having different types? That's going to break the HAL crate, and mean a lot more code in the PAC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The codegen gave the same types for the identical timers (timer1/2/3/4 for instance), but the type itself is different between different timers indeed :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's going to be a problem then, please keep the _derivedFrom
fixes. My HAL code depends on them being the same type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get issues when I try to use the derived peripherals:
TIMER0:
CTL0:
CEN:
Disabled: [0, "Counter disabled"]
Enabled: [1, "Counter enabled"]
"TIMER[1-9],TIMER1[0-9]":
CTL0:
CEN:
_derivedFrom: "TIMER0.CTL0.CEN.CEN"
If I try to use cen here for instance I get:
error[E0599]: no method named `disabled` found for struct `BitWriterRaw<'_, u32, gd32e1::gd32e103::timer1::ctl0::CTL0_SPEC, CEN_A, BitM, 0>` in the current scope
--> src/timer.rs:361:55
|
361 | self.timer.ctl0.modify(|_, w| w.cen().disabled());
| ^^^^^^^^ method not found in `BitWriterRaw<'_, u32, gd32e1::gd32e103::timer1::ctl0::CTL0_SPEC, CEN_A, BitM, 0>`
The read is working though (let is_counter_enabled = self.timer.ctl0.read().cen().is_enabled();
compiles)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried compiling the gd32f1x0-hal library on this repo's main branch and I got the same error 🤔
Same error using https://github.com/gd32-rust/gd32-rs-nightlies too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess I'm going to need to bisect then. I probably won't have time for a few days though. If you figure out what's going on in the meantime let me know.
If it would be helpful I'm happy to merge everything except for the timer changes for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a change that happened with the new svd2rust. During code generation the enum type and reader type are just linked to the original one, but the writer is put as a new type. The issue is either that the writer is defined as a new type or that the writer doesn't have any implementation (it should have the methods to set the register values). I'm trying to look in svd2rust's code to understand how this happened.
I don't need a merge right now, we can try and fix this and then merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found it, it appears to be a bug in svd2rust. This crate in v0.6.0 was using svd2rust 0.24.0 (which have been yanked from the registry), and the bug appeared in svd2rust 0.25.0.
I submitted an issue on their repo: rust-embedded/svd2rust#705
I've merged everything except for the timer changes in #41. I guess we need to wait for the fix to rust-embedded/svd2rust#705 before releasing anything. |
Thanks, I'm waiting for their answer, in the meantime I'll work with the duplicated version in the timers to implement the hal and will change back when the fix is implemented. |
Fixed by #51 |
There will be a lot a small (and some not so small) fixes on the GD32E1 PAC crate during the following days. I'm trying to put them all in this PR before I'll submit it to review